fix(purchase): suppress every variant when a legacy EC2 recommendation has no details - #1411

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/legacy-ec2-retry-suppressions
• File: .coderabbit.yaml
• Commit: bd044dd
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Essentials
• Run ID: e25303cc-867a-4d3b-a67b-c8a4e2bf8135

Commits

Reviewing files that changed from the base of the PR and between 6a4a506 and bd044dd.

Files selected for processing: 3

• internal/config/suppressions_build.go
• internal/config/suppressions_build_test.go
• internal/scheduler/suppression_variant_integration_test.go

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour.

Merge Risk: Minimal · up to bd044

The legacy retry path has no identified issue blocking merge.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the primary change: legacy EC2 recommendations without details now suppress every variant.
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

VERDICT: APPROVE bd044dd

PR #1411 (Part of #1232 item 2), 1 commit, 3 files, +64/-1. Fix is the one #1232 itself proposes ("treat empty EC2 Details as unknown, which means suppressing every variant").

Findings (none blocking):

• nit internal/config/suppressions_build_test.go:160-177 - nothing pins that an EC2 rec carrying only platform or only tenancy keeps its known variant (PR body says it does). Mutants &&->|| and dropping && v.Tenancy == "" both survive every config/scheduler/api/purchase test. If one landed, a current CE rec with nil Platform (tenancy is always set by resolveEC2Tenancy) would become a wildcard and over-suppress. That errs toward under-buying, so it is safe. Add a {"tenancy":"default"} case.
• nit internal/config/suppressions_build.go:45 - the wildcard drops the known top-level AZ as well (the CHECK in migration 000134 requires all three columns NULL or all set). A legacy us-east-1a rec therefore also suppresses the regional and other-AZ m5.large recs of the same 6-tuple until the grace period expires. This is under-buying only, it is what the issue's "suppress every variant" asks for, and it is bounded by expiry. Account, provider, service, region, resource type and engine all stay in the key.
• follow-up, outside this PR's scope: related legacy shapes the fix does not cover. (a) Details written before cloud-commitments-go 8888231 (2026-05-20) carry tenancy "shared". Their variant is {az, platform, "shared"}, which never matches today's "default" rec. (b) Rows from 2026-10-09 to 10-12 have the AZ only inside Details, with no top-level AvailabilityZone, so their variant is {"", ...} against today's zonal rec. Both windows are narrow (about 1 day and 3 days). The double-buy class is the same.

Attacked:

• No-details definitions: ComputeIdentity (recommendation_identity.go:30-42) maps nil, an empty slice, null, {} and an object with only unrelated keys to {"",""}, so all of these become wildcards. Details that do not decode take the existing err path, also a wildcard. The unit test covers nil, empty, null and {}.
• AppliesTo (types.go:765) returns true when s.Variant == nil. The store writes NULL in all three columns. The scheduler's matchingSuppressions (scheduler.go:1440) groups by 6-tuple, then applies. Migration 000134 documents an all-NULL row as a wildcard, with the CHECK all-or-none and UNIQUE NULLS NOT DISTINCT.
• Non-EC2: the guard is rec.Service == "ec2", and common.ServiceEC2 is "ec2". Mutation M3 (drop the guard) fails TestBuildSuppressions_NonComputeVariantIsKnownEmpty and TestApplySuppressions_NonComputeVariantMatches.
• Fresh purchase paths (handler_purchases.go:3564, plan_step_attach.go:348, attach_recommendations.go:35): the current CE parser always sets tenancy (parser_services.go:97), so fresh EC2 recs keep a known variant. No regression.
• Mutation probes: M1 (condition if false) fails the unit test and also the integration test (the later rec is not suppressed). M2 (||) survives. M3 fails. M4 (platform-only check) survives. The file was restored after each probe and diffed clean.
• Integration: -tags integration in ./internal/scheduler ran for real on testcontainers Postgres. TestLegacyEC2RetrySuppressesBoughtVariant_RealPostgres and TestSuppressionsScopedToPurchasedVariant_RealPostgres pass, and the new test fails under M1. It calls BuildSuppressions directly rather than going through the retry handler, which is acceptable because the handler only passes failedExec.Recommendations through (handler_purchases.go:2596).
• Unit: go test -run 'Suppress|AppliesTo' ./internal/config passes (22).
• CI at bd044dd: 31 pass, Security Scanning pending, no failures.

Independent review 2 (claude-fleet, cold): APPROVE bd044dd

Findings (none blocking):

• [non-blocking] internal/config/suppressions_build.go:45-51 - The wildcard also discards the AZ, which is known: it is a column, not part of Details.
  • Legacy EC2 recs in different AZs of one 6-tuple fall into a single NULL-variant bucket, with their counts summed.
  • A probe confirms it: 1a with count 2 plus 1b with count 3 gives one wildcard row of 5. That row subtracts 5 from every AZ, regional, platform and tenancy variant for the grace window.
  • This over-suppresses, which means under-buying, so it errs in the safe direction. The schema also has no row form for "AZ known, platform and tenancy wildcard".
  • The body does not mention the AZ widening or the summed counts.
• [non-blocking] suppressions_build.go:45 - EC2 Details that carry only one of the two fields ({"platform":"Linux/UNIX"} or {"tenancy":"dedicated"}) keep a narrow variant with one empty field.
  • No test pins this: M6 (the tenancy check dropped) survives the PR's tests.
  • The collector writes both fields, so the risk is low.
• [non-blocking] internal/scheduler/suppression_variant_integration_test.go - The new test seeds one rec and asserts Empty, so it never checks that the wildcard stops at the 6-tuple. A probe that covers this passes (below).
• [non-blocking, pre-existing, separate gap] internal/purchase/manager.go:575-584 says AWS recs spelled with the canonical slug "compute" also reach the EC2 client, but the new check matches only "ec2".
  • The suppression 6-tuple keys on service, so a "compute" suppression would not match "ec2" recs either way.
  • A legacy retry still fails at the EC2 client, which rejects an empty platform (execution.go:1153-1158). So its wildcard row hides the tuple for the whole grace window. That trade-off is intended.

Attacked:

1. Empty forms. All of these give Variant=nil:
   • nil, empty bytes, null and {};
   • {"platform":null,"tenancy":null} and {"platform":"","tenancy":""};
   • AZ-only Details, "\n{}\n" and " null ";
   • a regional legacy rec.
     Whitespace (" ") was already a wildcard on main, through the decode-error branch.
2. Unchanged cases. These keep the known empty variant {}: aws rds, elasticache with {}, azure compute, gcp compute with null, and aws "compute". EC2 with full Details keeps its exact variant.
3. Scheduler matching (scheduler.go:1439-1455, types.go:765-767). Rows are grouped by the 6-tuple, then filtered by AppliesTo. A scratch integration probe on real Postgres, with a legacy suppression of count 3:
   • The bought rec is hidden.
   • Windows/1b goes from 5 to 2, and regional dedicated from 4 to 1.
   • Another resource type, another region and RDS are untouched.
   • An expired legacy wildcard does not apply.
   • Result: PASS.
4. fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239 adds az_config only for database recs (RecIdentityKey and recommendationID) and leaves SuppressionVariant alone. It never touches the new EC2-only branch, and merge-tree of fix(recommendations): key stored recs, rec ID and purchase identity on RDS az_config (#408) #1239 with this head is clean.
5. Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
   • M1, the new branch disabled: the new unit and integration tests FAIL.
   • M2, applied to all services: NonComputeVariantIsKnownEmpty and the scheduler's NonComputeVariantMatches FAIL.
   • M3, && changed to ||: the same non-compute tests FAIL.
   • M4, && AZ=="" added: the new unit test FAILS.
   • M5, s.Variant==nil dropped in AppliesTo: AppliesTo and the scheduler's StrictAppliesSuppressions FAIL.
   • M6, a platform-only check: survives the PR's tests (the gap above).
6. Fail-before: on base, all 4 unit subtests FAIL, and the integration test FAILS with "the bought variant must be fully suppressed".

Verified locally, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

• Head:
  • go build ./...: OK.
  • go vet on config and scheduler, with and without -tags integration: OK.
  • go test -count=1 on config, scheduler, purchase and api: ok.
  • gofmt -l: clean.
• Merged tree:
  • git merge-tree --write-tree origin/main bd044dd: clean.
  • Build, vet and the same 4 packages pass on it, and so do the scheduler's RealPostgres integration tests.
• Testcontainers on head:
  • The full -tags integration ./internal/scheduler/ package: passes.
  • config -tags integration -run Suppress: ok.
• No migration.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

VERDICT: APPROVE b16fbbf

PR #1411, delta bd044dd..b16fbbf reviewed together with the full PR diff (2ded648..b16fbbf: suppressions_build.go, suppressions_build_test.go, scheduler/suppression_variant_integration_test.go).

Findings

1. (non-blocking, test gap) internal/config/suppressions_build.go:46 provider guard rec.Provider == "aws" is not pinned by any test. Mutant M2 (drop the guard) survives the full unit suites of internal/config, purchase, scheduler and api. Impact of a regression would be close to nil: Azure and GCP providers (pinned versions in go.mod:89-90) never set platform, tenancy or availability_zone, so their variant is always {"","",""} and a wildcard row matches exactly the same recs as a known-empty row (types.go:765 AppliesTo). Only the stored Variant column would differ. Suggest one assertion in TestBuildSuppressions_ComputeSlugEC2 that a gcp/azure "compute" rec keeps &SuppressionVariant{}.

2. (pre-existing, out of scope) Suppression matching is exact on service (scheduler.go:1416-1420 aggregateSuppressions, and the rec-side key in applySuppressionIndex). AWS recs collected by the scheduler carry "ec2" (providers/aws recommendations/client.go:368 calls GetRecommendationsForService(ServiceEC2); parser_ri.go:61 copies params.Service). A purchase request that names AWS EC2 as "compute" (validation.go:599 only trims the service; mapServiceType at purchase/execution.go:1222 normalises only for the purchase client, not the stored rec) writes a suppression under "compute" that will not hit the stored "ec2" recs, wildcard or not. This PR does not create or worsen that; it only makes the variant correct within whichever slug the rec carries. Worth a separate issue if the UI can send "compute" for AWS.

No blocking findings.

What I attacked and how

• Service spellings reaching BuildSuppressions: callers handler_purchases.go:2596 (retry, failedExec.Recommendations), handler_purchases.go:3564 (execute request), purchase/attach_recommendations.go:35, purchase/plan_step_attach.go:348. Request recs pass validatePurchaseRecommendation (validation.go:592-599): provider lowercased, service trimmed, so AWS EC2 can be "ec2" or "compute". Collected recs are "ec2". Both are now covered; matches the existing pairing in purchase/execution.go:1021 and handler_purchases.go:3412.
• Provider case: provider is lowercased on the request path and is common.ProviderAWS ("aws", pkg common/types.go:35) on the collector path. A hypothetical legacy "AWS" row would no longer wildcard, but its suppression already cannot match "aws" recs because the 6-tuple key is exact, so the guard does not worsen anything.
• Wildcard semantics: AppliesTo (types.go:765) returns true for nil Variant, so a legacy {az,"",""} retry suppresses the later Linux/UNIX variant; confirmed end to end by TestLegacyEC2RetrySuppressesBoughtVariant_RealPostgres.
• Partial details (only platform or only tenancy) keep the narrow variant: covered by the new subtests; mutant M3 proves it.
• Non-AWS compute: code path excluded by the guard (see finding 1 for test gap).

Mutation probes (scratch copy, reverted after each, diff confirmed identical)

• M1 drop || rec.Service == compute: TestBuildSuppressions_ComputeSlugEC2 FAILS.
• M2 drop provider guard: survives (finding 1).
• M3 && to || on platform/tenancy emptiness: TestBuildSuppressions_ComputeSlugEC2 FAILS.
• M4 provider "aws" replaced by "gcp": LegacyEC2WithoutDetailsIsWildcard and ComputeSlugEC2 FAIL.

Test evidence (macOS, scratch extract of b16fbbf, GOWORK=off)

• go build ./... and go vet ./...: OK
• go test -race -count=1 ./internal/config/... ./internal/purchase/...: ok config 2.0s, ok purchase 5.4s
• go test -tags integration -count=1 ./internal/config/...: ok 36.9s (testcontainers Postgres)
• go test -tags integration -run RealPostgres ./internal/scheduler/: TestSuppressionsScopedToPurchasedVariant_RealPostgres PASS, TestLegacyEC2RetrySuppressesBoughtVariant_RealPostgres PASS

Independent review 2 (claude-fleet, cold), re-review: APPROVE b16fbbf

This is a delta from my APPROVE at bd044dd, which covers one commit, b16fbbf (+33/-3).

The change: internal/config/suppressions_build.go:46-48 now treats a rec as AWS EC2 when Provider == aws and Service is ec2 or the canonical compute slug.

• That fixes the gap I noted: an AWS rec spelled "compute" with empty Details now gets the wildcard too.
• The new provider condition also narrows the old check, so an Azure or GCP "compute" rec keeps its known empty variant and is not widened into a wildcard. That is correct.
• TestBuildSuppressions_ComputeSlugEC2 pins three cases:
  • compute with empty Details gives a wildcard;
  • platform-only keeps {az, platform, ""};
  • tenancy-only keeps {az, "", tenancy}.
  • The last two close my earlier M6 gap.

Probes on a scratch copy, each restored and confirmed identical with cmp:

• The compute slug dropped from the predicate: TestBuildSuppressions_ComputeSlugEC2 FAILS.
• The tenancy check dropped (my earlier M6): TestBuildSuppressions_ComputeSlugEC2 FAILS.
• The Provider == aws condition dropped: survives (follow-up below).

Follow-up (non-blocking): no test pins the new provider condition.

• If it regressed, an Azure or GCP "compute" rec with empty Details would silently become a wildcard, and suppress every variant of its tuple for the grace window.
• That errs toward over-suppressing (under-buying), the safe direction.
• One more case would pin it: provider azure, service compute, empty Details, asserting a known {az,"",""} variant.

Unchanged: the separate, older gap remains. The suppression 6-tuple keys on service, so a "compute" suppression still does not match an "ec2"-spelled rec, or the reverse. This PR does not change that.

Verified locally on a git archive export of b16fbbf, with GOTOOLCHAIN=go1.26.10 and no cloud credentials, under the heavy lock:

• git merge-tree --write-tree origin/main b16fbbf: clean.
• go build ./... and go vet ./internal/config/ ./internal/scheduler/: ok. gofmt -l internal/config: clean.
• go test -count=1 ./internal/config/ ./internal/scheduler/ ./internal/purchase/: ok.
• go test -tags integration -run 'Suppress|RealPostgres' ./internal/config/ ./internal/scheduler/: ok.

Same reviewer as the earlier review, re-reviewing the new head. Review only: nothing was pushed, and the GitHub review state is unchanged.

Local verification at b16fbbf967b2c8c5a20fdc3b2efc9b2b7b678058 merged onto current main (26004a6d; merge-tree ea9998b3; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• go test -tags integration passes on real Postgres (testcontainers) for the touched packages: ./internal/config ./internal/scheduler
• Nothing was applied to any cloud.